-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alternative 1: Suffix renamings (no suffix) #437
Conversation
Well, I never liked the _u32 suffix, so agreed on that ;-) For parameters that are known never to be negative, I would prefer either unsigned int or unsigned long. The difference is that unsigned long is guaranteed to contain at least 32 bit (even on MP-16BIT). So, if the API is expected to handle at least 32-bits on all platforms then unsigned long is better, otherwise unsigned int is better IMHO. Thanks for doing this! |
@nijtmans I also never liked a suffix for those functions (but I like them for the getters/setters). Regarding unsigned - for now I want to keep it as int, like many other functions (eg mp_mul_2d etc). I want to separate the issues. |
Yes, for the getters/setters it's OK, agreed!
OK, understood! I'm aware of the downsides: It sure will have effect on the code, e.g. loops. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! B.T.W. the only function from this group used by Tcl is mp_expt_u32, so as long as this function can handle at least 28 bits for argument b it's fine for me.
@nijtmans I assume on 32/64 bit archs? Why 28 exactly? Is this some magic width in tcl due to unboxed integers in the language runtime? |
I don't know why the limit is exactly 28, but there is a test-case in Tcl's test-suite which checks whether pow(2^28) fails. Just try in Tcl (any version):
Such exponentiation takes incredibly long to calculate ... So I see the reason for limiting the exponent to 28 bits, even with 27 bits it could be used for a DOS attack on Tcl (don't tell this to any hackers group, please ....) |
The exponentiation itself doesn't, it shouldn't take more than a couple of seconds, it is the number conversion that takes aeons. Even a well optimized program like Pari/GP needs over 15 seconds to convert But don't worry, we are working at it in #330. |
Indeed:
A fraction of a second. |
@sjaeckel your opinion on this? Do you have a better proposal? |
548463c
to
d232821
Compare
d232821
to
cc0d360
Compare
cc0d360
to
438f2db
Compare
closed in favor of #446 |
Moved out from #434.
Repeating what I wrote in #434:
Renaming some mp_(root|expt|log)_u32 functions back to suffix less version. Using uint32_t for those functions was a mistake, I have to admit that now. In particular using a suffix is not a good idea, since this is a slight obstacle now to change the types :( These functions should either take/return bitcounts (int) or mp_digits. For example mp_log is defined in terms of count_bits, so this is the natural type to use. mp_root calls mp_mul_d, so mp_digit would be a good fit. For now I am using simply int.
We still have to decide if we want a new suffix!
Furthermore we might want to discuss renaming
mp_div_2d -> mp_rsh, mp_mul_2d -> mp_lsh
since the 2d suffix is weird. It would also be more consistent withmp_signed_rsh
. Alternatively something likemp_div_2expt
might make sense. But we can also keep things as is regarding these two functions.